Fix GtkApplicationWindow action group implementation

GtkApplicationWindow frees its internal action group on dispose for the
usual reasons: to avoid the possibility of reference cycles caused by
actions referring back to the window again.

Unfortunately, if it happens to be inside of a GtkActionMuxer at the
time that it is disposed, it will (eventually) be removed from the muxer
after it has been disposed.  Removing an action group from a muxer
involves a call to g_action_group_list_actions() which will crash
because the internal action group to which we normally delegate the call
has been freed.

A future patch that reworks the quartz menu code will introduce a use of
GtkActionMuxer in a way that causes exactly this problem.

We can guard against the problem in a number of ways.

First, we can avoid the entire situation by ensuring that we are removed
from the muxer before we destroy the action group.  To this end, we
delay destruction of the action group until after the chain-up to the
dispose of GtkWindow (which is where the window is removed from the
GtkApplication).

Secondly, we can add checks to each of our GActionGroup and GActionMap
implementation functions to check that the internal action group is
still alive before we attempt to delegate to it.

We have to be careful, though: because our _list_actions() call will
suddenly be returning an empty list, people watching the group from
outside will have expected to see "action-removed" calls for the
now-missing items.  Make sure we send those. but only if someone is
watching.

https://bugzilla.gnome.org/show_bug.cgi?id=710351

diff --git a/gtk/gtkapplicationwindow.c b/gtk/gtkapplicationwindow.c
index 5cbe71e03ff448c1bdec041df9983cdf1dc9d5ed..df337c0b4d8c02614d414398245fe7ec638db480 100644 (file)
--- a/gtk/gtkapplicationwindow.c
+++ b/gtk/gtkapplicationwindow.c
@@ -389,6 +389,10 @@ gtk_application_window_list_actions (GActionGroup *group)
 {
   GtkApplicationWindow *window = GTK_APPLICATION_WINDOW (group);
 
+  /* may be NULL after dispose has run */
+  if (!window->priv->actions)
+    return g_new0 (char *, 0 + 1);
+
   return g_action_group_list_actions (G_ACTION_GROUP (window->priv->actions));
 }
 
@@ -403,6 +407,9 @@ gtk_application_window_query_action (GActionGroup        *group,
 {
   GtkApplicationWindow *window = GTK_APPLICATION_WINDOW (group);
 
+  if (!window->priv->actions)
+    return FALSE;
+
   return g_action_group_query_action (G_ACTION_GROUP (window->priv->actions),
                                       action_name, enabled, parameter_type, state_type, state_hint, state);
 }
@@ -414,7 +421,10 @@ gtk_application_window_activate_action (GActionGroup *group,
 {
   GtkApplicationWindow *window = GTK_APPLICATION_WINDOW (group);
 
-  return g_action_group_activate_action (G_ACTION_GROUP (window->priv->actions), action_name, parameter);
+  if (!window->priv->actions)
+    return;
+
+  g_action_group_activate_action (G_ACTION_GROUP (window->priv->actions), action_name, parameter);
 }
 
 static void
@@ -424,7 +434,10 @@ gtk_application_window_change_action_state (GActionGroup *group,
 {
   GtkApplicationWindow *window = GTK_APPLICATION_WINDOW (group);
 
-  return g_action_group_change_action_state (G_ACTION_GROUP (window->priv->actions), action_name, state);
+  if (!window->priv->actions)
+    return;
+
+  g_action_group_change_action_state (G_ACTION_GROUP (window->priv->actions), action_name, state);
 }
 
 static GAction *
@@ -433,6 +446,9 @@ gtk_application_window_lookup_action (GActionMap  *action_map,
 {
   GtkApplicationWindow *window = GTK_APPLICATION_WINDOW (action_map);
 
+  if (!window->priv->actions)
+    return NULL;
+
   return g_action_map_lookup_action (G_ACTION_MAP (window->priv->actions), action_name);
 }
 
@@ -442,6 +458,9 @@ gtk_application_window_add_action (GActionMap *action_map,
 {
   GtkApplicationWindow *window = GTK_APPLICATION_WINDOW (action_map);
 
+  if (!window->priv->actions)
+    return;
+
   g_action_map_add_action (G_ACTION_MAP (window->priv->actions), action);
 }
 
@@ -451,6 +470,9 @@ gtk_application_window_remove_action (GActionMap  *action_map,
 {
   GtkApplicationWindow *window = GTK_APPLICATION_WINDOW (action_map);
 
+  if (!window->priv->actions)
+    return;
+
   g_action_map_remove_action (G_ACTION_MAP (window->priv->actions), action_name);
 }
 
@@ -740,10 +762,59 @@ gtk_application_window_dispose (GObject *object)
 
   g_clear_object (&window->priv->app_menu_section);
   g_clear_object (&window->priv->menubar_section);
-  g_clear_object (&window->priv->actions);
 
   G_OBJECT_CLASS (gtk_application_window_parent_class)
     ->dispose (object);
+
+  /* We do this below the chain-up above to give us a chance to be
+   * removed from the GtkApplication (which is done in the dispose
+   * handler of GtkWindow).
+   *
+   * That reduces our chances of being watched as a GActionGroup from a
+   * muxer constructed by GtkApplication.  Even still, it's
+   * theoretically possible that someone else could be watching us.
+   * Therefore, we have to take care to ensure that we don't violate our
+   * obligations under the interface of GActionGroup.
+   *
+   * The easiest thing is just for us to act as if all of the actions
+   * suddenly disappeared.
+   */
+  if (window->priv->actions)
+    {
+      gchar **action_names;
+      guint signal;
+      gint i;
+
+      /* Only send the remove signals if someone is listening */
+      signal = g_signal_lookup ("action-removed", G_TYPE_ACTION_GROUP);
+      if (signal && g_signal_has_handler_pending (window, signal, 0, TRUE))
+        /* need to send a removed signal for each action */
+        action_names = g_action_group_list_actions (G_ACTION_GROUP (window->priv->actions));
+      else
+        /* don't need to send signals: nobody is watching */
+        action_names = NULL;
+
+      /* Free the group before sending the signals for two reasons:
+       *
+       *  1) we want any incoming calls to see an empty group
+       *
+       *  2) we don't want signal handlers that trigger in response to
+       *     the action-removed signals that we're firing to attempt to
+       *     modify the action group in a way that may cause it to fire
+       *     additional signals (which we would then propagate)
+       */
+      g_object_unref (window->priv->actions);
+      window->priv->actions = NULL;
+
+      /* It's safe to send the signals now, if we need to. */
+      if (action_names)
+        {
+          for (i = 0; action_names[i]; i++)
+            g_action_group_action_removed (G_ACTION_GROUP (window), action_names[i]);
+
+          g_strfreev (action_names);
+        }
+    }
 }
 
 static void